Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a comprehensive actions API to VortexJS that enables keyboard shortcuts and action management across the framework. The implementation provides a context-based action system where components can register actions with keyboard shortcuts that are automatically handled.
Key changes:
- New actions API with context provider and hooks for action registration
- DOM keyboard action handler that listens for key combinations and triggers registered actions
- Configuration updates to support the new JSX import source and path mapping
Reviewed Changes
Copilot reviewed 13 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/vortex-core/src/actions/index.tsx | Core actions API with types, context, and hooks |
| packages/vortex-dom/src/actions/DOMActions.tsx | DOM keyboard event handler for action shortcuts |
| packages/vortex-core/src/std/abort.ts | Utility hook for abort signal management |
| packages/vortex-core/src/render/index.tsx | Integration of ActionProvider into render pipeline |
| packages/vortex-dom/src/index.ts | Export of actions from DOM package |
| packages/vortex-core/package.json | Build configuration update for actions module |
| packages/vortex-*/tsconfig.json | TypeScript configuration updates |
| packages/example-wormhole/src/features/home/index.tsx | Example usage of the actions API |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const shortcut = Array.from(keys).sort().map(x => getCodeToKey(x)); | ||
| keys.delete(ev.code); | ||
|
|
||
| let sortedShortcut = sortKeys(shortcut); |
There was a problem hiding this comment.
The shortcut array is being sorted before mapping to keys, but then sorted again with sortKeys(). This creates inconsistent ordering since the first sort is alphabetical while sortKeys() uses a custom order. The mapping should happen before any sorting.
| const shortcut = Array.from(keys).sort().map(x => getCodeToKey(x)); | |
| keys.delete(ev.code); | |
| let sortedShortcut = sortKeys(shortcut); | |
| const shortcut = sortKeys(Array.from(keys).map(x => getCodeToKey(x))); | |
| keys.delete(ev.code); | |
| let sortedShortcut = shortcut; |
| const action = getImmediateValue(actions).find(x => | ||
| x.shortcut && sortKeys(x.shortcut.split("+")).join("+") === shortcutString); |
There was a problem hiding this comment.
The sortKeys() function is called on every action's shortcut during each keyup event. Consider pre-processing and storing normalized shortcuts when actions are registered to avoid repeated computation.
| const action = getImmediateValue(actions).find(x => | |
| x.shortcut && sortKeys(x.shortcut.split("+")).join("+") === shortcutString); | |
| const action = actionsWithNormalizedShortcuts.find(x => | |
| x.normalizedShortcut === shortcutString); |
| window.addEventListener("keydown", (ev) => { | ||
| keys.add(ev.code); | ||
| }, { signal }); | ||
|
|
||
| window.addEventListener("keyup", (ev) => { |
There was a problem hiding this comment.
The keyboard handler doesn't respect user preferences for reduced motion or accessibility settings. Consider checking if the user has disabled keyboard shortcuts or if the current element should receive keyboard events.
| window.addEventListener("keydown", (ev) => { | |
| keys.add(ev.code); | |
| }, { signal }); | |
| window.addEventListener("keyup", (ev) => { | |
| window.addEventListener("keydown", (ev) => { | |
| // Check if user has disabled keyboard shortcuts | |
| const shortcutsEnabled = !window.localStorage.getItem("disableKeyboardShortcuts"); | |
| // Check if the focused element should receive keyboard events | |
| const active = document.activeElement; | |
| const isEditable = active && ( | |
| active.tagName === "INPUT" || | |
| active.tagName === "TEXTAREA" || | |
| (active as HTMLElement).isContentEditable | |
| ); | |
| if (!shortcutsEnabled || isEditable) return; | |
| keys.add(ev.code); | |
| }, { signal }); | |
| window.addEventListener("keyup", (ev) => { | |
| // Check if user has disabled keyboard shortcuts | |
| const shortcutsEnabled = !window.localStorage.getItem("disableKeyboardShortcuts"); | |
| // Check if the focused element should receive keyboard events | |
| const active = document.activeElement; | |
| const isEditable = active && ( | |
| active.tagName === "INPUT" || | |
| active.tagName === "TEXTAREA" || | |
| (active as HTMLElement).isContentEditable | |
| ); | |
| if (!shortcutsEnabled || isEditable) return; |
No description provided.